Skip to content
This repository has been archived by the owner on Jan 8, 2025. It is now read-only.

doclets: Allow overloaded functions to not conflict #111

Closed
wants to merge 2 commits into from

Conversation

tavianator
Copy link
Contributor

Fixes #110.

@erikrose
Copy link
Contributor

  1. This is a significant enough change that it requires new tests to demonstrate its efficacy.
  2. If there are multiple functions at the same pathname, shouldn't we, at minimum, favor the documented one, if any? It looks like the error you were experiencing happened only when trying to actually use the docs; correct me if I'm wrong. (Of course, the ultimate would be to be able to somehow specify which instance of the function we wanted, but that would need much more design work.)

@erikrose
Copy link
Contributor

I forgot to say it, but thanks for the patch! :-) I don't use TS myself, and nobody's maintaining sphinx-js's support at the moment, so I'm very keen to have your interest.

@tavianator
Copy link
Contributor Author

Sure, I'm happy to add some tests for it.

The error I was getting happens when building the docs. With the new behaviour, both overloads are listed in the docs, which is probably what you want (TypeDoc also lists both, but only one at a time, like this: https://typedoc.org/api/classes/application.html#generatedocs). Here's what the result looks like in sphinx-js: https://bistring.readthedocs.io/en/latest/JavaScript/Alignment.html#Alignment.originalBounds

The tree that reports the conflicts is used to resolve links I believe. So when you write something like :js:meth:`Alignment.originalBounds` , it has to resolve that to the actual element to link to. This change makes it just pick the first overload arbitrarily. I'm open to other behaviours here, or other ways of rendering overloads.

You're welcome for the patch! :)

@graup
Copy link
Contributor

graup commented Oct 1, 2019

Tested it quickly, seems to work. This is definitely much better than throwing errors. Let's get some tests for this and merge it!

@tavianator
Copy link
Contributor Author

@graup Added a simple test!

erikrose
erikrose previously approved these changes Oct 1, 2019
Copy link
Contributor

@erikrose erikrose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, and thanks for the test! :-D

Copy link
Contributor

@erikrose erikrose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I have to go make sure that it still throws an error if conflicting functions are found in plain-JS. That's a useful signal for when you need to go back and dismabiguate your references. Bbiab....

@tavianator
Copy link
Contributor Author

@erikrose The code I changed doesn't distinguish between vanilla JS and TypeScript, so I'm pretty sure it won't. There's another issue as well: if you do something like .. js:autofunction:: overloaded it will only render one of them. I guess the tree needs to hold a list of matching elements rather than just one.

@erikrose erikrose dismissed their stale review October 2, 2019 01:13

Trouble emerged.

Copy link
Contributor

@erikrose erikrose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so apparently we have more thinking to do here.

I'll leave it to you and weigh in once you come up with a design you feel good about. Feel free to let config flags be a part of it if necessary; that's fine with me as long as all nontrivial combinations of state have good test coverage.

I have regrets about the state of TS support in sphinx-js. The original contributor gave us a large piece of useful, well-tested code, but it hooked up TS by converting typedoc's output into the ill-specified, oft-changing JSON format jsdoc -X emits, because that's what sphinx-js operated on directly at the time (and still does). Had we talked during the design process, I'd have suggested we define a stable intermediate representation and write everything to that. But I was a teensy bit lazy in the 1.0 days, and now it's one of those temporary-begets-permanent situations. I'd still be happy to rewrite the JS parts to hook up to an IR—I think it would be borderline trivial—but I'm not prepared to tackle TS, as typedoc.py is full of branches and a mystery to me. Applicants welcome. :-)

@graup
Copy link
Contributor

graup commented Oct 2, 2019

@erikrose I'd be happy to contribute. Should we open a new issue to design an IR and coordinate working on a branch together?

There are also some problems with some other ts features currently not supported by the jsdoc representation that we could fix then.

@tavianator
Copy link
Contributor Author

I think an IR would be nice. Would enable us to have TS-specific things in the documentation as first-class citizens.

To avoid needing to mess with typedoc.py at first, since we already have TypeDoc -> JSDoc, we could start with just doing TypeDoc -> JSDoc -> IR. Then later rewrite the conversion to just do TypeDoc -> IR, and compare the results on some corpus of existing TypeScript projects.

I don't have a tremendous amount of time to contribute to this but I'm happy to review designs and implementations, and maybe do some of the TS side of it.

@erikrose
Copy link
Contributor

erikrose commented Oct 7, 2019

Alright, I started doing some of the initial spec work toward an IR over at #120. Help is welcome!

@erikrose
Copy link
Contributor

erikrose commented Sep 2, 2020

It doesn't crash anymore as of the great IR merge of #120, demonstrated by

def test_array(self):
"""Make sure array types are rendered correctly.
As a bonus, make sure we grab the first signature of an overloaded
function.
"""
obj = self.analyzer.get_object(['overload'])
assert obj.params[0].type == 'string[]'
. But we should probably show multiple signatures if the Sphinx templates will let us. I've opened #153 to represent that work.

@erikrose erikrose closed this Sep 2, 2020
hoodmane added a commit to hoodmane/sphinx-js that referenced this pull request Apr 23, 2024
This uses a decorator to call post_convert after each call to `to_ir` so that it
can't be forgotten. In order to allow subclasses to restrict the type of
`to_ir`, we use a decorator (I originally tried a similar approach as with
`type.render_name` but it doesn't work with the subclass type restrictions).
Typing the decorator took a bit of trial and error, but the key thing is to say
that the decorator doesn't change the type of the function.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript overloads fail
3 participants